Skip to content

refactor(handlers): use vim.system where possible and anchor lua patterns#35

Closed
antinomie8 wants to merge 4 commits intoyousefakbar:mainfrom
antinomie8:refactor/handlers-cleanup
Closed

refactor(handlers): use vim.system where possible and anchor lua patterns#35
antinomie8 wants to merge 4 commits intoyousefakbar:mainfrom
antinomie8:refactor/handlers-cleanup

Conversation

@antinomie8
Copy link
Copy Markdown
Contributor

The title and commit messages should be self-explanatory. Currently, we check whether command is a table but call the unrecommended vim.fn.system in both cases; that's probably a mistake (both if branches do the exact same thing), so I rewrote it to use vim.system when command is a table, else fallback to vim.fn.system.
I also added anchors to string.match patterns where they made sense.

Other stuff related to handlers

  • To decide which handler to use, we match the mimetype and the extension. I don't really see the point on checking the extension since the mimetype is more robust and precise anyways, so I think it could be considered to eventually remove extension matching.
  • It is possible for the user to override the default_view_handler function altogether, but as I pointed out in feat(view): add support for viewing calendar file attachments #33, it might be beneficial to let the user add custom mimetype pattern matching to lua callback for extending the default instead of overriding it. Let me know if you want me to create a PR for that.

@antinomie8 antinomie8 changed the title Use vim.system when possible and anchor lua patterns refactor(handlers): use vim.system where possible and anchor lua patterns Jan 21, 2026
@yousefakbar
Copy link
Copy Markdown
Owner

Hi @anonymousgrasshopper . This change looks good to me. Silly of me to not have caught that weird double vim.fn.system() call so thanks for catching that!

I left one review on how success is determined. I think it's better to rely on the command's return code instead of stderr because some programs may fail but not print anything to stderr, so it's safer to rely on return code IMO.

It is possible for the user to override the default_view_handler function altogether, but as I pointed out in feat(view): add support for viewing calendar file attachments #33, it might be beneficial to let the user add custom mimetype pattern matching to lua callback for extending the default instead of overriding it. Let me know if you want me to create a PR for that.

100% this is a pain point for me right now. As observed in #33 the current handler logic -- while it works great -- is lacking in extensibility because if you want to "add one more handler for a certain filetype", then the user has to override and re-write their own comprehensive handler logic which is tedious.

I'm thinking of refactoring the handler logic into making it modular with a pattern-based registry where handlers are maintained in a map/dictionary like { ['text/html'] = function(attachment) logic end } so that the default is still available, but the user can override existing filetype handlers or add new ones modularly without re-implementing the entire handler function.

I'll raise an issue on this so it can be brainstormed further and planned for the next release.

@yousefakbar
Copy link
Copy Markdown
Owner

Thanks for the fixes! Merged to main and closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants